Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(NcCheckboxRadioSwitch): Pass attrs to input if available #5507

Merged
merged 1 commit into from
Apr 20, 2024
Merged

fix(NcCheckboxRadioSwitch): Pass attrs to input if available #5507

merged 1 commit into from
Apr 20, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Apr 19, 2024

☑️ Resolves

I would expect the attrs on the input element to allow any aria attributes.
So if not button type, pass the attrs to the input element to allow setting aria tags like aria-invalid and aria-errormessage

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux susnux added bug Something isn't working 3. to review Waiting for reviews feature: checkbox-radio-switch Related to the checkbox-radio-switch component labels Apr 19, 2024
@susnux
Copy link
Contributor Author

susnux commented Apr 19, 2024

/backport to next

@susnux
fix(NcCheckboxRadioSwitch): Pass attrs to input if available
395fc5f 
If not button type, pass the attrs to the input element to allow setting aria tags like `aria-invalid` and `aria-errormessage`

Signed-off-by: Ferdinand Thiessen <[email protected]>
ShGKme
ShGKme approved these changes Apr 20, 2024
View reviewed changes
Copy link
Contributor

@ShGKme ShGKme left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't pass to the button as well?

@susnux
Copy link
Contributor Author

susnux commented Apr 20, 2024

Why don't pass to the button as well?

They are as in this case it is the root element.

@susnux
Copy link
Contributor Author

susnux commented Apr 20, 2024

we maybe should add consistent props for all input elements to have some focus and validation API independent of the implementation.

@susnux susnux merged commit d6d00ca into master Apr 20, 2024
18 checks passed
@susnux susnux deleted the fix/checkbox-attrs branch April 20, 2024 15:39
@backportbot backportbot bot mentioned this pull request Apr 20, 2024
Merged
2 tasks
ShGKme
ShGKme reviewed Apr 21, 2024
View reviewed changes
src/components/NcCheckboxRadioSwitch/NcCheckboxRadioSwitch.vue
Comment on lines +329 to +330
// We need to pass attributes to the input element
inheritAttrs: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are as in this case it is the root element.

But it is disabled via inheritAttrs: false here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For vue2 class and style are passed to the root as they are not attrs but somewhat special

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://v3-migration.vuejs.org/breaking-changes/attrs-includes-class-style#_2-x-behavior

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't pass to the button as well?

Here I meant the attrs, not only class/style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there is a new possibility to pass attrs to input, but the possibility to pass attrs to the button has been disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No for button type they are passed:
https://github.com/nextcloud-libraries/nextcloud-vue/pull/5507/files#diff-80b068a320cd09d28f4f8913f4c77c73754bed31f69a9b3a25353059d8573623R279

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So class & style are always set to the wrapper.
attrs are passed to the wrapper (the button) for button type and to the input for other types.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, missed this line.

@Antreesy Antreesy mentioned this pull request May 7, 2024
Merged
@Antreesy Antreesy added this to the 8.11.3 milestone May 7, 2024
This was referenced Jul 2, 2024
Merged
Merged
@skjnldsv
Copy link
Contributor

skjnldsv commented Jul 3, 2024 •
edited
Loading

Breaks server nextcloud/server#46239
we can't apply attributes to the outer span now, that is an issue imho

@skjnldsv skjnldsv mentioned this pull request Jul 4, 2024
Merged
@susnux
Copy link
Contributor Author

susnux commented Jul 4, 2024

we can't apply attributes to the outer span now, that is an issue imho

The structure of components is private and can change anytime, thus assumptions about it will always be prone to break.
That is why we have e.g. content-classes etc for assigning custom classes to dialogs without any assumption about internal class names or structure.
In this case you have a checkbox thus you would expect to have a checkbox element (input).

This "regression" is only about tests that assume some specific internal structure, but it is more a fix.

If you currently have cy.get('[data-cy-my-checkbox] input') you could simplify to cy.get('[data-cy-my-checkbox]') or the most secure way: cy.findByRole('checkbox', { name: 'My checkbox' }) which will work regardless of the internal structure and only depends on your app code (the accessible name that you assign).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShGKme ShGKme ShGKme approved these changes

@raimund-schluessler raimund-schluessler raimund-schluessler approved these changes

@Pytal Pytal Awaiting requested review from Pytal

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug Something isn't working feature: checkbox-radio-switch Related to the checkbox-radio-switch component
Projects
None yet
Milestone
8.11.3
Development

Successfully merging this pull request may close these issues.
5 participants
@susnux @skjnldsv @raimund-schluessler @ShGKme @Antreesy